test: add Pest v1 security test infrastructure#217
Conversation
Add source-scan tests verifying security patterns (prepared statements, output escaping, auth guards, PHP 7.4 compatibility) remain in place across refactors. Tests run with Pest v1 (PHP 7.3+) and stub the Cacti framework so plugins can be tested in isolation. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a Pest v1-based test scaffold intended to validate security hardening patterns and basic plugin structure via lightweight source-scanning tests.
Changes:
- Introduces a
composer.jsonwith Pest as a dev dependency for running the new test suite. - Adds a Pest bootstrap with Cacti framework function stubs to support isolated test execution.
- Adds security-oriented tests for
setup.phpstructure, DB prepared-statement usage consistency, and avoidance of a few PHP 8-only APIs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
composer.json |
Adds Pest v1 dev dependency and test bootstrap autoloading. |
tests/bootstrap.php |
Stubs key Cacti DB/helpers/constants for test execution outside Cacti. |
tests/Pest.php |
Loads the test bootstrap for Pest runs. |
tests/Security/SetupStructureTest.php |
Verifies setup.php declares expected plugin hook functions and version keys. |
tests/Security/PreparedStatementConsistencyTest.php |
Scans selected files for raw db_* calls vs prepared helpers. |
tests/Security/Php74CompatibilityTest.php |
Scans selected files for a few PHP 8.0+ APIs/operators. |
| describe('prepared statement consistency in weathermap', function () { | ||
| it('uses prepared DB helpers in all plugin files', function () { | ||
| $targetFiles = array( | ||
| 'check.php', | ||
| 'setup.php', | ||
| ); | ||
|
|
||
| $rawPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)\s*\(/'; | ||
| $preparedPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)_prepared\s*\(/'; |
There was a problem hiding this comment.
This test will currently fail against the existing repo: setup.php contains multiple raw db_execute(...) / db_fetch_*( calls (e.g., setup.php:72-76, setup.php:137, setup.php:593). Either migrate those calls to the prepared helpers, or narrow/adjust this test (e.g., exclude DDL/static SQL sections or limit the scan to files that have actually been migrated).
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The continue branches when realpath()/file_get_contents() fails cause the test to silently skip files and still pass, reducing its value as a regression check. Since check.php/setup.php are expected to exist, consider failing the test with a clear message if a target file can't be located or read.
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect($path)->not->toBeFalse("Expected test target file {$relativeFile} to exist"); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse("Expected test target file {$relativeFile} to be readable"); |
| $lines = explode("\n", $contents); | ||
| $rawCallsOutsideComments = 0; | ||
|
|
||
| foreach ($lines as $line) { | ||
| $trimmed = ltrim($line); | ||
|
|
||
| if (strpos($trimmed, '//') === 0 || strpos($trimmed, '*') === 0 || strpos($trimmed, '#') === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (preg_match($rawPattern, $line) && !preg_match($preparedPattern, $line)) { | ||
| $rawCallsOutsideComments++; | ||
| } | ||
| } | ||
|
|
||
| expect($rawCallsOutsideComments)->toBe(0, |
There was a problem hiding this comment.
The comment-skipping logic only ignores lines starting with //, *, or #. It will still count patterns inside /* ... */ lines (trimmed starts with /*) and also inside inline comments after code, which can create false positives. Consider using token_get_all() to ignore PHP comments/strings, or at least expand the filter to handle /* and inline comment segments.
| $lines = explode("\n", $contents); | |
| $rawCallsOutsideComments = 0; | |
| foreach ($lines as $line) { | |
| $trimmed = ltrim($line); | |
| if (strpos($trimmed, '//') === 0 || strpos($trimmed, '*') === 0 || strpos($trimmed, '#') === 0) { | |
| continue; | |
| } | |
| if (preg_match($rawPattern, $line) && !preg_match($preparedPattern, $line)) { | |
| $rawCallsOutsideComments++; | |
| } | |
| } | |
| expect($rawCallsOutsideComments)->toBe(0, | |
| $tokens = token_get_all($contents); | |
| $searchableCode = ''; | |
| foreach ($tokens as $token) { | |
| if (is_array($token)) { | |
| if ( | |
| $token[0] === T_COMMENT || | |
| $token[0] === T_DOC_COMMENT || | |
| $token[0] === T_CONSTANT_ENCAPSED_STRING || | |
| $token[0] === T_INLINE_HTML | |
| ) { | |
| continue; | |
| } | |
| $searchableCode .= $token[1]; | |
| } else { | |
| $searchableCode .= $token; | |
| } | |
| } | |
| $hasRawCalls = preg_match($rawPattern, $searchableCode) && !preg_match($preparedPattern, $searchableCode); | |
| expect($hasRawCalls)->toBeFalse( |
| it('does not use str_contains (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, | ||
| "{$relativeFile} uses str_contains() which requires PHP 8.0" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('does not use str_starts_with (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, | ||
| "{$relativeFile} uses str_starts_with() which requires PHP 8.0" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('does not use str_ends_with (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, | ||
| "{$relativeFile} uses str_ends_with() which requires PHP 8.0" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('does not use nullsafe operator (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests continue when realpath() or file_get_contents() fails, which can make the suite pass even if the target source files are missing/unreadable. Since these files are required for the guarantee you're testing, consider failing with a clear assertion when a target file can't be found/read.
| it('does not use str_contains (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_contains() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_starts_with (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_starts_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_ends_with (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_ends_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use nullsafe operator (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| $get_file_contents = function (string $relativeFile): string { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| expect($path)->not->toBeFalse("Unable to resolve required source file: {$relativeFile}"); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse("Unable to read required source file: {$relativeFile}"); | |
| return $contents; | |
| }; | |
| it('does not use str_contains (PHP 8.0)', function () use ($files, $get_file_contents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $get_file_contents($relativeFile); | |
| expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_contains() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_starts_with (PHP 8.0)', function () use ($files, $get_file_contents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $get_file_contents($relativeFile); | |
| expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_starts_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_ends_with (PHP 8.0)', function () use ($files, $get_file_contents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $get_file_contents($relativeFile); | |
| expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_ends_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use nullsafe operator (PHP 8.0)', function () use ($files, $get_file_contents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $get_file_contents($relativeFile); |
| * Verify plugin source files do not use PHP 8.0+ syntax. | ||
| * Cacti 1.2.x plugins must remain compatible with PHP 7.4. | ||
| */ | ||
|
|
||
| describe('PHP 7.4 compatibility in weathermap', function () { |
There was a problem hiding this comment.
The header comment asserts the plugin must remain compatible with PHP 7.4, but this repo’s GitHub Actions workflow tests PHP 8.1–8.4 only (.github/workflows/plugin-ci-workflow.yml:41). If the project no longer targets PHP 7.4, update the comment (and potentially rename the test) to match the supported PHP range so the intent is accurate.
| * Verify plugin source files do not use PHP 8.0+ syntax. | |
| * Cacti 1.2.x plugins must remain compatible with PHP 7.4. | |
| */ | |
| describe('PHP 7.4 compatibility in weathermap', function () { | |
| * Verify key plugin entry-point source files do not use PHP 8.0-only syntax. | |
| * This is a narrow syntax guard for these files and does not define overall | |
| * plugin runtime support beyond the repository's current PHP test matrix. | |
| */ | |
| describe('PHP syntax guardrails in weathermap', function () { |
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | ||
|
|
||
| it('defines plugin_weathermap_install function', function () use ($source) { | ||
| expect($source)->toContain('function plugin_weathermap_install'); | ||
| }); | ||
|
|
||
| it('defines plugin_weathermap_version function', function () use ($source) { | ||
| expect($source)->toContain('function plugin_weathermap_version'); | ||
| }); | ||
|
|
||
| it('defines plugin_weathermap_uninstall function', function () use ($source) { | ||
| expect($source)->toContain('function plugin_weathermap_uninstall'); | ||
| }); | ||
|
|
||
| it('returns version array with name key', function () use ($source) { | ||
| expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/'); | ||
| }); | ||
|
|
||
| it('returns version array with version key', function () use ($source) { |
There was a problem hiding this comment.
file_get_contents(realpath(...)) is executed at describe evaluation time; if the path resolution/read fails, you’ll get warnings and the whole file may error before any assertions run. Consider asserting the path/content are valid (or loading in a beforeAll/inside each it) so failures are reported cleanly.
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | |
| it('defines plugin_weathermap_install function', function () use ($source) { | |
| expect($source)->toContain('function plugin_weathermap_install'); | |
| }); | |
| it('defines plugin_weathermap_version function', function () use ($source) { | |
| expect($source)->toContain('function plugin_weathermap_version'); | |
| }); | |
| it('defines plugin_weathermap_uninstall function', function () use ($source) { | |
| expect($source)->toContain('function plugin_weathermap_uninstall'); | |
| }); | |
| it('returns version array with name key', function () use ($source) { | |
| expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/'); | |
| }); | |
| it('returns version array with version key', function () use ($source) { | |
| $get_source = function () { | |
| $setup_path = realpath(__DIR__ . '/../../setup.php'); | |
| expect($setup_path)->not->toBeFalse(); | |
| $source = file_get_contents($setup_path); | |
| expect($source)->not->toBeFalse(); | |
| return $source; | |
| }; | |
| it('defines plugin_weathermap_install function', function () use ($get_source) { | |
| $source = $get_source(); | |
| expect($source)->toContain('function plugin_weathermap_install'); | |
| }); | |
| it('defines plugin_weathermap_version function', function () use ($get_source) { | |
| $source = $get_source(); | |
| expect($source)->toContain('function plugin_weathermap_version'); | |
| }); | |
| it('defines plugin_weathermap_uninstall function', function () use ($get_source) { | |
| $source = $get_source(); | |
| expect($source)->toContain('function plugin_weathermap_uninstall'); | |
| }); | |
| it('returns version array with name key', function () use ($get_source) { | |
| $source = $get_source(); | |
| expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/'); | |
| }); | |
| it('returns version array with version key', function () use ($get_source) { | |
| $source = $get_source(); |
| { | ||
| "name": "cacti/plugin_weathermap", | ||
| "description": "plugin_weathermap plugin for Cacti", | ||
| "license": "GPL-2.0-or-later", |
There was a problem hiding this comment.
Consider adding an explicit PHP version constraint (e.g., in require or config.platform) aligned with the versions this repo supports/tests. Without it, composer install behavior can vary by developer machine PHP version and fail with less actionable solver errors.
| "license": "GPL-2.0-or-later", | |
| "license": "GPL-2.0-or-later", | |
| "require": { | |
| "php": "^8.1" | |
| }, |
…dabot - Throw RuntimeException when realpath/file_get_contents fails (previously silent continue hid unscanned files) - Fix Dependabot ecosystem from npm to composer - Remove committed .omc session artifacts, add .omc/ to .gitignore Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Converted to draft to serialize the stack in this repo. Blocked by #215; will un-draft after that merges to avoid cross-PR merge conflicts. |
Summary
Test plan
composer install && vendor/bin/pestpasses